Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle Cygwin / Git Bash sockets forwarding on Windows #82

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

amurzeau
Copy link
Contributor

This is used to forward ssh-agent connection to Git Bash' ssh-agent.

I'm not sure this is the cleanest way to implement this, but I've not found existing library functions that could reduce the amount of code.

I've checked that the forwarding work using a "test" and a pipe server to be able to test this case:
ssh-add => \\.\pipe\mypipe => test => call connectLocal to forward to Git Bash's ssh-agent socket

A way to test with VS Code would be better than using a manual test, but I don't how I could integrate it in VS Code.

I've also tried to test the connection to gpg-agent (via the socket file named S.gpg-agent.ssh), but I couldn't make it works, despite expected packets are sent to gpg-agent according to wireshark. It seems that something broke on gpg-agent side according to:

  • https://github.com/rupor-github/win-gpg-agent

    Assuan S.gpg-agent.ssh support in GnuPG code is presently broken under Windows (at least in GnuPG 2.2.25), so we have to resort to putty/pageant method instead

  • https://dev.gnupg.org/T3883#113453

    It appears that the actual ssh-agent message coming across the bridge is never read or acted upon, but the socket connection isn't closed despite the log message stating that the handler is terminated.

  • https://dev.gnupg.org/T3883#150791

    With Assuan S.gpg-agent.ssh support in GnuPG code was broken under Windows, [...]

  • https://dev.gnupg.org/T3883#150854

    Also last time I looked for whatever reasons Assuan S.gpg-agent.ssh support in GnuPG code was broken under Windows (there was a comment in the code on that)


Here is the explanation of what is required to connect to a cygwin / git bash unix domain socket on Windows:

  • Port parsing:
    • Git Bash' unix sockets requires connecting to the port whose number is in the socket file along with a cookie.
    • The socket file contains something like !<socket >63488 s 44693F4F-E2572CA5-537862AB-248DFDEF
    • The port here is 63488 and the cookie is 44693F4F-E2572CA5-537862AB-248DFDEF.
    • So I retrieve the port and cookie using a regex and convert it to a number.
      • If the file content does not match the regex, I assume this is a GPG socket and use the existing code to parse it.
    • When I have the port and the cookie, I connect to 127.0.0.1:<port>, then I do the following handshake.
  • Cygwin / Git Bash socket Handshake:
    • The handshake consists in:
    • the client must send the cookie as 16 raw bytes
      - The cookie is formatted in the socket file as 4 32 bits hex integers. They must be send to the ssh-agent server in little endian as 16 raw bytes (this means according to the above example: 0x4F 0x3F 0x69 0x44 0xA5 0x72 ...).
    • the server send back the same 16 bytes if the cookie is valid, else closes the connection (so the client must skip these 16 bytes, as done in skipHeader)
    • the client must send pid and user id and user effective id information in a 12 bytes packet
      • I set the pid to a real value from process.pid, but ssh-agent ignores it
      • user id and user effective id are both set to 0
    • the server send back the same information, but about the server, I just ignore these 12 bytes too in skipHeader; this is a function that just skip the handshake data).

As the server send back data in the handshake phase (16 + 12 bytes), I need to skip them through the use of skipHeader.

Then actual data transfer can take place.

See also:
https://stackoverflow.com/questions/23086038/what-mechanism-is-used-by-msys-cygwin-to-emulate-unix-domain-sockets
https://github.com/abourget/secrets-bridge/blob/094959a1553943e0727f6524289e12e8aab697bf/pkg/agentfwd/agentconn_windows.go#L15

Fix: #62

@chrmarti chrmarti self-assigned this Jul 15, 2022
@chrmarti chrmarti added this to the July 2022 milestone Jul 15, 2022
@chrmarti
Copy link
Contributor

@amurzeau Thanks for your PR! Code looks good. I will need to do some more testing myself (expect some delay as I will have some time off). My (older) GPG4Win install still worked.

@bamurtaugh
Copy link
Member

@cla-bot check

@amurzeau
Copy link
Contributor Author

amurzeau commented Nov 7, 2022

I think I will need to sign the CLA, but I don't see any link to click on, maybe not ready yet ?

@bamurtaugh
Copy link
Member

@amurzeau sorry for any confusion, yes we're still investigating. Thanks for your patience!

@msftgits msftgits requested a review from a team as a code owner November 7, 2022 20:04
@amurzeau
Copy link
Contributor Author

amurzeau commented Nov 7, 2022

@microsoft-github-policy-service agree

Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, left a few comments. Thanks!

src/spec-common/cliHost.ts Outdated Show resolved Hide resolved
src/spec-common/cliHost.ts Show resolved Hide resolved
src/spec-common/cliHost.ts Outdated Show resolved Hide resolved
@amurzeau amurzeau force-pushed the cygwin-socket-forwarding branch from 1cfd99b to 9e65800 Compare November 10, 2022 00:08
Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, left one more question regarding the updated error handling.

src/spec-common/cliHost.ts Outdated Show resolved Hide resolved
This is used to forward ssh-agent connection to Git Bash' ssh-agent.

Here is the explanation of what is required to connect to a cygwin / git
bash unix domain socket on Windows:

- Port parsing:

  - Git Bash' unix sockets requires connecting to the port whose number
    is in the socket file along with a cookie.

  - The socket file contains something like `!<socket >63488 s 44693F4F-E2572CA5-537862AB-248DFDEF`

  - The port here is `63488` and the cookie is `44693F4F-E2572CA5-537862AB-248DFDEF`.

  - So I retrieve the port and cookie using a regex and convert it to a
    number.

    - If the file content does not match the regex, I assume this is a
      GPG socket and use the existing code to parse it.

  - When I have the port and the cookie, I connect to `127.0.0.1:<port>`,
    then I do the following handshake.

- Cygwin / Git Bash socket Handshake:

  -  The handshake consists in:

    -  the client must send the cookie as 16 raw bytes

      - The cookie is formatted in the socket file as 4 32 bits hex
        integers. They must be send to the ssh-agent server in little
        endian as 16 raw bytes (this means according to the above
        example: `0x4F 0x3F 0x69 0x44  0xA5 0x72 ...`).

    -  the server send back the same 16 bytes if the cookie is valid,
       else closes the connection (so the client must skip these 16
       bytes, as done in `skipHeader`)

    - the client must send pid and user id and user effective id
      information in a 12 bytes packet

      - I set the pid to a real value from process.pid, but ssh-agent
        ignores it

      - user id and user effective id are both set to 0

    - the server send back the same information, but about the server, I
      just ignore these 12 bytes too in `skipHeader`; this is a function
      that just skip the handshake data).

As the server send back data in the handshake phase (16 + 12 bytes), I
need to skip them through the use of `skipHeader`.

Then actual data transfer can take place.

See also:
https://stackoverflow.com/questions/23086038/what-mechanism-is-used-by-msys-cygwin-to-emulate-unix-domain-sockets
https://github.com/abourget/secrets-bridge/blob/094959a1553943e0727f6524289e12e8aab697bf/pkg/agentfwd/agentconn_windows.go#L15

Fix: devcontainers#62
@amurzeau amurzeau force-pushed the cygwin-socket-forwarding branch from 9e65800 to bd684d6 Compare November 11, 2022 18:38
@chrmarti chrmarti requested a review from a team November 14, 2022 08:35
Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! (Will ask for a second review and then merge.)

@chrmarti chrmarti merged commit 6ebc652 into devcontainers:main Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cliHost.ts: connectLocal function and cygwin/mingw unix socket files
4 participants